Extract reusable classes from flow metrics #18502
Conversation
🤖 GitHub commentsJust comment with:
|
|
This pull request does not have a backport label. Could you fix it @andsel? 🙏
|
andsel
changed the title
There was a problem hiding this comment.
Pull request overview
This PR extracts reusable classes from the ExtendedFlowMetric implementation to enable future support for histogram flow metrics. The refactoring moves the RetentionWindow inner class to a standalone class and creates a new DatapointCapture base class to generalize capture types beyond just FlowCapture.
Key changes:
- Introduced
DatapointCaptureas an abstract base class for point-in-time metric data with timing information - Refactored
FlowCaptureto extendDatapointCapture, moving common nanoTime handling to the base class - Extracted
RetentionWindowfrom an inner class to a standalone package-private class that works withDatapointCaptureinstead of justFlowCapture
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
DatapointCapture.java |
New abstract base class providing timing information and capture selection logic for all datapoint types |
FlowCapture.java |
Refactored to extend DatapointCapture, removing duplicated nanoTime field and method |
RetentionWindow.java |
Extracted from ExtendedFlowMetric as a standalone class, generalized to work with DatapointCapture instead of FlowCapture |
ExtendedFlowMetric.java |
Removed inner RetentionWindow class and selectNewerCapture utility method, added cast when consuming baseline captures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .map((baseline) -> calculateRate(currentCapture, (FlowCapture) baseline)) | ||
| .orElseGet(OptionalDouble::empty) | ||
| .ifPresent((rate) -> rates.put(window.policy.policyName(), rate))); |
There was a problem hiding this comment.
The baseline() method now returns Optional, but this code unconditionally casts it to FlowCapture. While this may work in the current implementation since RetentionWindow is only used with FlowCapture instances, this cast could fail at runtime if RetentionWindow is later used with other DatapointCapture subtypes. Consider either: 1) making RetentionWindow generic over the capture type, or 2) adding a type check before the cast with appropriate error handling.
| .map((baseline) -> calculateRate(currentCapture, (FlowCapture) baseline)) | |
| .orElseGet(OptionalDouble::empty) | |
| .ifPresent((rate) -> rates.put(window.policy.policyName(), rate))); | |
| .flatMap(baseline -> { | |
| if (baseline instanceof FlowCapture) { | |
| return calculateRate(currentCapture, (FlowCapture) baseline); | |
| } else { | |
| LOGGER.warn("RetentionWindow baseline is not a FlowCapture for policy '{}'; skipping rate calculation", window.policy.policyName()); | |
| return OptionalDouble.empty(); | |
| } | |
| }) | |
| .ifPresent(rate -> rates.put(window.policy.policyName(), rate))); |
logstash-core/src/main/java/org/logstash/instrument/metrics/DatapointCapture.java
Outdated
Show resolved
Hide resolved
logstash-core/src/main/java/org/logstash/instrument/metrics/RetentionWindow.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
logstash-core/src/main/java/org/logstash/instrument/metrics/DatapointCapture.java
Outdated
Show resolved
Hide resolved
| if (b instanceof FlowCapture baseline) { | ||
| return calculateRate(currentCapture, baseline); | ||
| } else { | ||
| LOGGER.warn("Retention window `{}` provided a non-FlowCapture baseline `{}`; skipping rate calculation", | ||
| window.policy.policyName(), b); | ||
| return OptionalDouble.empty(); | ||
| } |
There was a problem hiding this comment.
I' not sure exactly what the right abstraction is, but needing to use instanceof here in the middle of this chain when FlowCapture is the only concrete implementation of DatapointCapture feels like we're setting ourselves up for leaky encapsulation.
The logic here (mapping retention windows to values) is very tightly-coupled with the nature of flow metrics, which as rate-of-change metrics relies pretty heavily on the baseline/comparison shortcuts, while I know that the proposed histogram metrics won't be using. I took a pretty big stab at generalizing this a few months ago (to make supporting your histogram work possible), but was unable at the time to find an abstraction that actually encapsulates both. While there are certainly parallels between the two, the implementation details of flow metrics clash with those of the histograms.
There was a problem hiding this comment.
I understand your point, so I would circle back on the meaning of the retention window. The retention window is a concurrent list of datapoints, so I imagined it as container of those. The characteristic of datapoints is that are values of measures taken at a specific time, so that compaction and expiration logic (drop and remove from the list) can be applied.
If we move some aggregation logic specific to flow metric or histogram computation outside of the container then we could get a reusable data structure. For example, FlowMetric uses the baseline while the HdrHistogramFlowMetric in #18503 leverage an iteration method forEachCapture. If we move these logic outside in a class that has responsibility to compute something out of raw data, then we could reuse the code. WDYT?
There was a problem hiding this comment.
With 2b8ca9a I tried to make RetentionWindow a container of subclasses of DatapointCapture. Now the ExtendedFlowMetric declare to use RetentionWindows of FlowCaptures and with this spirit in #18503 the HistogramSnapshotsWindow will declare to use RetentionWindow of HistogramCapture. In that PR the forEachCapture will remain in the RetentionWindow as legit way to iterate over the data points, unless we decide that RetentionWindow is an Iterable.
…asses od DatapointCapture. - Added type paramters with a constrant to be a subclass of DatapointCapture which is something that have a datapoint capture time - Peeled out baseline method into ExtendedFlowMetric, it's a wrapper on returnHead(nanos)
💚 Build Succeeded
History
cc @andsel |
3 participants
Release notes
[rn:skip]
What does this PR do?
Extract common code from datapoint that can be used by the
RetentionWindowto be used to compute a flow metric.Moves RetentionWindow
ExtendedFlowMetric's inner class to be standalone class usable by other metric implementations.Why is it important/What is the impact to the user?
As a developer of histogram flow metric I want to reuse existing code used to implement ExtendedFlowMetric.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files (and/or docker env variables)[ ] I have added tests that prove my fix is effective or that my feature worksAuthor's Checklist
How to test this PR locally
Related issues